Fix for non shippable products in Product Doctor#6318
Merged
Conversation
🦋 Changeset detectedLatest commit: 60737e4 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes false positive shipping zone warnings in the Product Doctor diagnostics for non-shippable products (digital goods, activation codes, etc.). The fix adds the isShippingRequired field from the product type to diagnostic data and conditionally skips shipping checks for products that don't require shipping.
Changes:
- Added
isShippingRequiredfield to GraphQL fragments and queries to fetch shipping requirement status from product types - Modified diagnostic logic to skip shipping zone checks for non-shippable products while maintaining warehouse/stock checks
- Added comprehensive test coverage for both shippable and non-shippable product scenarios
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/fragments/products.ts | Added isShippingRequired field to product type fragment |
| src/graphql/hooks.generated.ts | Updated generated GraphQL hook to include new field |
| src/graphql/types.generated.ts | Updated generated types with isShippingRequired in ProductFragment and ProductDetailsQuery |
| src/products/fixtures.ts | Added isShippingRequired: true to fixture and removed incorrectly placed taxClass from productType |
| src/products/components/ProductDoctor/utils/types.ts | Added isShippingRequired field to ProductDiagnosticData interface with comprehensive documentation |
| src/products/components/ProductDoctor/utils/mapProductToDiagnosticData.ts | Maps isShippingRequired from product type with safe default to true |
| src/products/components/ProductDoctor/utils/mapProductToDiagnosticData.test.ts | Added comprehensive tests for mapping logic including edge cases |
| src/products/components/ProductDoctor/utils/availabilityChecks.ts | Modified to skip shipping checks for non-shippable products, includes minor performance optimization |
| src/products/components/ProductDoctor/utils/availabilityChecks.test.ts | New comprehensive test file covering shippable/non-shippable scenarios |
| src/products/components/ProductDoctor/hooks/useProductAvailabilityDiagnostics.test.tsx | Updated mock to include new isShippingRequired field |
| .changeset/long-lions-ring.md | Changeset documenting the fix |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6318 +/- ##
==========================================
+ Coverage 42.59% 42.79% +0.19%
==========================================
Files 2497 2497
Lines 43401 43405 +4
Branches 9851 10232 +381
==========================================
+ Hits 18485 18573 +88
+ Misses 24879 23516 -1363
- Partials 37 1316 +1279 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mirekm
changed the title
lkostrowski
previously approved these changes
Feb 5, 2026
witoszekdev
reviewed
Feb 6, 2026
Comment on lines
+6
to
+37
| // Mock intl for testing - validates that message placeholders are provided | ||
| const createMockIntl = (): IntlShape => | ||
| ({ | ||
| formatMessage: (descriptor: MessageDescriptor, values?: Record<string, unknown>) => { | ||
| const message = String(descriptor.defaultMessage ?? ""); | ||
|
|
||
| // Extract placeholders like {channelName} from the message | ||
| const placeholders: string[] = message.match(/\{(\w+)\}/g) ?? []; | ||
|
|
||
| // Verify all placeholders have corresponding values | ||
| placeholders.forEach((placeholder: string) => { | ||
| const key = placeholder.slice(1, -1); // Remove { and } | ||
|
|
||
| if (!values || !(key in values)) { | ||
| throw new Error(`Missing value for placeholder "${key}" in message: "${message}"`); | ||
| } | ||
| }); | ||
|
|
||
| // Replace placeholders with values for readable test output | ||
| let result = message; | ||
|
|
||
| if (values) { | ||
| Object.entries(values).forEach(([key, value]) => { | ||
| result = result.replace(new RegExp(`\\{${key}\\}`, "g"), String(value)); | ||
| }); | ||
| } | ||
|
|
||
| return result; | ||
| }, | ||
| }) as unknown as IntlShape; | ||
|
|
||
| const mockIntl = createMockIntl(); |
Member
There was a problem hiding this comment.
suggestion: if we need to have checks in tests for proper formatting of placeholders, maybe we should just use the actual react-intl with a mocked provider?
i'm not sure if we need this though, since we are not making any checks for messages in the actual tests below if I understand correctly. In that case we can make this much simpler:
Suggested change
| // Mock intl for testing - validates that message placeholders are provided | |
| const createMockIntl = (): IntlShape => | |
| ({ | |
| formatMessage: (descriptor: MessageDescriptor, values?: Record<string, unknown>) => { | |
| const message = String(descriptor.defaultMessage ?? ""); | |
| // Extract placeholders like {channelName} from the message | |
| const placeholders: string[] = message.match(/\{(\w+)\}/g) ?? []; | |
| // Verify all placeholders have corresponding values | |
| placeholders.forEach((placeholder: string) => { | |
| const key = placeholder.slice(1, -1); // Remove { and } | |
| if (!values || !(key in values)) { | |
| throw new Error(`Missing value for placeholder "${key}" in message: "${message}"`); | |
| } | |
| }); | |
| // Replace placeholders with values for readable test output | |
| let result = message; | |
| if (values) { | |
| Object.entries(values).forEach(([key, value]) => { | |
| result = result.replace(new RegExp(`\\{${key}\\}`, "g"), String(value)); | |
| }); | |
| } | |
| return result; | |
| }, | |
| }) as unknown as IntlShape; | |
| const mockIntl = createMockIntl(); | |
| const mockIntl = { | |
| formatMessage: (descriptor: MessageDescriptor) => return descriptor?.defaultMessage ?? ""; | |
| } as unknown as IntlShape; |
src/products/components/ProductDoctor/utils/mapProductToDiagnosticData.test.ts
Outdated
Show resolved
Hide resolved
src/products/components/ProductDoctor/utils/mapProductToDiagnosticData.test.ts
Outdated
Show resolved
Hide resolved
src/products/components/ProductDoctor/utils/mapProductToDiagnosticData.test.ts
Outdated
Show resolved
Hide resolved
src/products/components/ProductDoctor/utils/mapProductToDiagnosticData.ts
Outdated
Show resolved
Hide resolved
mirekm
changed the title
witoszekdev
approved these changes
Feb 9, 2026
This was referenced Feb 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Product availability diagnostics should skip shipping zone warnings for non-shippable products (digital goods, activation codes, etc.). Products with isShippingRequired: false on their product type will no longer see false positive warnings about missing shipping zones or unreachable warehouses via shipping.